Automated Test: oauth-state-secure #330
Conversation
…#67876) We're adding one more step in the GitHub integration installation pipeline, namely GitHub OAuth2 authorize. This is transparent from the UX perspective as the data exchange happens without user interaction. The pipeline will now fail in these cases: - If there is a mismatch between currently authenticated GitHub user (derived from OAuth2 authorize step) and the user who installed the GitHub app (https://github.com/apps/sentry-io) - If there is a mismatch between `state` parameter supplied by user and pipeline signature - If GitHub could not generate correct `access_token` from the `code` (wrong or attempt of re-use of `code`). In all those cases, this error is shown: 
📝 WalkthroughWalkthroughThese changes implement an OAuth-based login flow for GitHub integration setup, introducing an OAuthLoginView class that handles GitHub authorization before installation. The implementation includes error handling utilities, updates to pipeline views to include the new OAuth step, removal of a multi-provider constant in the pipeline advancer, and corresponding test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OAuthLoginView
participant GitHub
participant Pipeline
participant GitHubInstallation
Client->>OAuthLoginView: initiate setup
OAuthLoginView->>OAuthLoginView: generate state & redirect_uri
OAuthLoginView-->>Client: redirect to GitHub authorize URL
Client->>GitHub: authorize with client_id & state
GitHub-->>Client: redirect with authorization code
Client->>OAuthLoginView: callback with code & state
OAuthLoginView->>OAuthLoginView: validate state
OAuthLoginView->>GitHub: exchange code for access_token
GitHub-->>OAuthLoginView: return access_token
OAuthLoginView->>GitHub: fetch user info
GitHub-->>OAuthLoginView: return user info
OAuthLoginView->>Pipeline: store authenticated username
OAuthLoginView-->>Pipeline: advance to next step
Pipeline->>GitHubInstallation: process installation
GitHubInstallation-->>Client: installation complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/sentry/integrations/github/test_integration.py (1)
378-396:⚠️ Potential issue | 🟠 Major
test_installation_not_foundis testing state validation failure, not an installation lookup failure.The test sets up a 404 mock on the
/app/installations/{id}endpoint (line 381-383), but this code path is never reached. The hardcoded stateddd023d87a913d5226e2a882c4c4cc05(line 392) does not match the pipeline's signature9cae5e88803f35ed7970fc131e6e65d3(established insetUp()and verified inassert_setup_flow()). This causesOAuthLoginView.dispatch()to fail the state validation check on line 412 ofintegration.pyand returnerror()with the default message "Invalid installation request." before attempting the installation lookup.The test assertion passes, but it's validating the wrong scenario. To properly test the 404 case, the state parameter must match the pipeline signature (as shown in
assert_setup_flow()on line 242).
🤖 Fix all issues with AI agents
In `@src/sentry/integrations/github/integration.py`:
- Around line 425-436: The call to get_user_info(payload["access_token"]) can
raise network or API errors but isn't wrapped, causing 500s; wrap the
get_user_info invocation (and any subsequent access to authenticated_user_info)
in a try/except, return the friendly error(request, self.active_organization) on
failure, and log the exception (include the token context minimally) so failures
in get_user_info are handled gracefully; update the block that currently decodes
body/safely parses payload and the later "if 'login' not in
authenticated_user_info" check to use the protected result.
- Around line 493-505: The code assumes integration.metadata["sender"]["login"]
always exists and can raise KeyError; update the check around
pipeline.fetch_state("github_authenticated_user") to safely access
integration.metadata by verifying "sender" exists and has a "login" (e.g., check
"sender" in integration.metadata and "login" in integration.metadata["sender"])
or use a helper/get with default None, then only compare when present; reference
the Integration lookup (Integration.objects.get), the integration.metadata
access, and pipeline.fetch_state("github_authenticated_user") and fall back to
returning error(request, self.active_organization) if sender/login is missing or
does not match.
🧹 Nitpick comments (1)
src/sentry/integrations/github/integration.py (1)
404-408: Query parameters in the redirect URL are not URL-encoded.The
redirect_urivalue contains characters like:and/that should be percent-encoded when used as a query parameter value. While GitHub's OAuth endpoint likely handles this gracefully, it's not standards-compliant and could break with stricter URL parsers.Consider using
urlencode(already imported viaparse_qsl's module) to construct the query string:♻️ Proposed fix
+from urllib.parse import parse_qsl, urlencode ... - return self.redirect( - f"{ghip.get_oauth_authorize_url()}?client_id={github_client_id}&state={state}&redirect_uri={redirect_uri}" - ) + params = urlencode({ + "client_id": github_client_id, + "state": state, + "redirect_uri": redirect_uri, + }) + return self.redirect( + f"{ghip.get_oauth_authorize_url()}?{params}" + )
| try: | ||
| body = safe_urlread(req).decode("utf-8") | ||
| payload = dict(parse_qsl(body)) | ||
| except Exception: | ||
| payload = {} | ||
|
|
||
| if "access_token" not in payload: | ||
| return error(request, self.active_organization) | ||
|
|
||
| authenticated_user_info = get_user_info(payload["access_token"]) | ||
| if "login" not in authenticated_user_info: | ||
| return error(request, self.active_organization) |
There was a problem hiding this comment.
Missing error handling for get_user_info — unhandled exceptions will 500.
The token exchange on lines 425-429 is wrapped in a try/except, but the get_user_info call on line 434 is unprotected. If the GitHub /user API call fails (network error, rate limit, invalid token), this will raise an unhandled exception and return a 500 to the user instead of the friendly error page.
🐛 Proposed fix
- authenticated_user_info = get_user_info(payload["access_token"])
- if "login" not in authenticated_user_info:
+ try:
+ authenticated_user_info = get_user_info(payload["access_token"])
+ except Exception:
+ return error(request, self.active_organization)
+
+ if "login" not in authenticated_user_info:
return error(request, self.active_organization)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| body = safe_urlread(req).decode("utf-8") | |
| payload = dict(parse_qsl(body)) | |
| except Exception: | |
| payload = {} | |
| if "access_token" not in payload: | |
| return error(request, self.active_organization) | |
| authenticated_user_info = get_user_info(payload["access_token"]) | |
| if "login" not in authenticated_user_info: | |
| return error(request, self.active_organization) | |
| try: | |
| body = safe_urlread(req).decode("utf-8") | |
| payload = dict(parse_qsl(body)) | |
| except Exception: | |
| payload = {} | |
| if "access_token" not in payload: | |
| return error(request, self.active_organization) | |
| try: | |
| authenticated_user_info = get_user_info(payload["access_token"]) | |
| except Exception: | |
| return error(request, self.active_organization) | |
| if "login" not in authenticated_user_info: | |
| return error(request, self.active_organization) |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 428-428: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In `@src/sentry/integrations/github/integration.py` around lines 425 - 436, The
call to get_user_info(payload["access_token"]) can raise network or API errors
but isn't wrapped, causing 500s; wrap the get_user_info invocation (and any
subsequent access to authenticated_user_info) in a try/except, return the
friendly error(request, self.active_organization) on failure, and log the
exception (include the token context minimally) so failures in get_user_info are
handled gracefully; update the block that currently decodes body/safely parses
payload and the later "if 'login' not in authenticated_user_info" check to use
the protected result.
| try: | ||
| integration = Integration.objects.get( | ||
| external_id=installation_id, status=ObjectStatus.ACTIVE | ||
| ) | ||
| except Integration.DoesNotExist: | ||
| return error(request, self.active_organization) | ||
|
|
||
| # Check that the authenticated GitHub user is the same as who installed the app. | ||
| if ( | ||
| pipeline.fetch_state("github_authenticated_user") | ||
| != integration.metadata["sender"]["login"] | ||
| ): | ||
| return error(request, self.active_organization) |
There was a problem hiding this comment.
Potential KeyError if integration metadata lacks a "sender" key.
Line 503 accesses integration.metadata["sender"]["login"] without checking whether the "sender" key exists. The build_integration method (line 376) only sets metadata["sender"] conditionally when state.get("sender") is present. For integrations created without a sender (e.g., older installations or webhook-created ones), this will raise a KeyError and return a 500 error.
🐛 Proposed fix
try:
integration = Integration.objects.get(
external_id=installation_id, status=ObjectStatus.ACTIVE
)
except Integration.DoesNotExist:
return error(request, self.active_organization)
+ sender_login = (integration.metadata or {}).get("sender", {}).get("login")
# Check that the authenticated GitHub user is the same as who installed the app.
if (
pipeline.fetch_state("github_authenticated_user")
- != integration.metadata["sender"]["login"]
+ != sender_login
):
return error(request, self.active_organization)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| integration = Integration.objects.get( | |
| external_id=installation_id, status=ObjectStatus.ACTIVE | |
| ) | |
| except Integration.DoesNotExist: | |
| return error(request, self.active_organization) | |
| # Check that the authenticated GitHub user is the same as who installed the app. | |
| if ( | |
| pipeline.fetch_state("github_authenticated_user") | |
| != integration.metadata["sender"]["login"] | |
| ): | |
| return error(request, self.active_organization) | |
| try: | |
| integration = Integration.objects.get( | |
| external_id=installation_id, status=ObjectStatus.ACTIVE | |
| ) | |
| except Integration.DoesNotExist: | |
| return error(request, self.active_organization) | |
| sender_login = (integration.metadata or {}).get("sender", {}).get("login") | |
| # Check that the authenticated GitHub user is the same as who installed the app. | |
| if ( | |
| pipeline.fetch_state("github_authenticated_user") | |
| != sender_login | |
| ): | |
| return error(request, self.active_organization) |
🤖 Prompt for AI Agents
In `@src/sentry/integrations/github/integration.py` around lines 493 - 505, The
code assumes integration.metadata["sender"]["login"] always exists and can raise
KeyError; update the check around
pipeline.fetch_state("github_authenticated_user") to safely access
integration.metadata by verifying "sender" exists and has a "login" (e.g., check
"sender" in integration.metadata and "login" in integration.metadata["sender"])
or use a helper/get with default None, then only compare when present; reference
the Integration lookup (Integration.objects.get), the integration.metadata
access, and pipeline.fetch_state("github_authenticated_user") and fall back to
returning error(request, self.active_organization) if sender/login is missing or
does not match.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes
New Features
Improvements